Skip to content

[CORRUPTED] Synthetic Benchmark PR #103102 - feat(tracemetrics): Parse metric from aggregations #16

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
tomerqodo wants to merge 5 commits into
base: base_pr_103102_20251204_6705
Choose a base branch
from
Open

[CORRUPTED] Synthetic Benchmark PR #103102 - feat(tracemetrics): Parse metric from aggregations #16

tomerqodo wants to merge 5 commits into from

Conversation

@tomerqodo
Copy link

@tomerqodo tomerqodo commented Dec 4, 2025 •
edited by qodo-code-review bot
Loading

User description

Benchmark PR getsentry#103102

Type: Corrupted (contains bugs)

Original PR Title: feat(tracemetrics): Parse metric from aggregations
Original PR Description: This parses the metric from aggregations instead of relying on the top level metric name/type. The top level ones still take precedence for better compatibility. It has the limitation that it only allows a single metric across all the aggregations.
Original PR URL: getsentry#103102

PR Type

Enhancement

Description

  • Parse metric metadata from aggregation functions instead of relying solely on top-level metric parameters

  • Extract metric name, type, and unit from selected columns and equations to automatically filter trace metrics

  • Refactor metric filtering logic into reusable utility functions and configuration methods

  • Add validation to prevent querying multiple different metrics in a single request

Diagram Walkthrough

flowchart LR
  A["Aggregation Functions"] -->|extract metric metadata| B["ResolvedMetricAggregate"]
  C["Selected Columns/Equations"] -->|resolve and parse| B
  B -->|validate single metric| D["Metric Filter"]
  E["Top-level Config"] -->|fallback to| D
  D -->|apply conditions| F["Query Execution"]
Loading

File Walkthrough
Relevant files
Enhancement
columns.py
Add metric metadata extraction to aggregates                         

src/sentry/search/eap/columns.py

  • Add ResolvedMetricAggregate dataclass to store metric metadata (name,
    type, unit) extracted from aggregations
  • Import cast utility and MetricType from types module
  • Modify TraceMetricAggregateDefinition.resolve() to extract and return
    metric information from aggregation arguments
  • Update return type from ResolvedAggregate to ResolvedMetricAggregate
    with populated metric fields
 +24/-6   
resolver.py
Refactor query resolution with dataset conditions               

src/sentry/search/eap/resolver.py

  • Extract environment query merging logic into and_trace_item_filters()
    utility function
  • Add new resolve_query_with_columns() method that calls
    resolve_dataset_conditions() to inject dataset-specific filters
  • Implement resolve_dataset_conditions() to delegate to config's
    extra_conditions() method with selected columns and equations
  • Simplify resolve_query() to use the new utility function for filter
    merging
 +35/-22 
rpc_utils.py
Add trace item filter utility functions                                   

src/sentry/search/eap/rpc_utils.py

  • Create new utility module with and_trace_item_filters() function
  • Implement filter merging logic that handles None values and combines
    multiple filters with AND logic
  • Support combining 1 or more filters into a single TraceItemFilter with
    AndFilter
 +14/-0   
config.py
Extract metrics from aggregations with fallback                   

src/sentry/search/eap/trace_metrics/config.py

  • Move MetricType definition from local to types.py module
  • Create Metric dataclass to represent metric with name, type, and unit
  • Refactor extra_conditions() to accept selected_columns and equations
    parameters
  • Add _extra_conditions_from_metric() to handle top-level metric config
    filtering
  • Add _extra_conditions_from_columns() to extract metrics from
    aggregation functions and equations
  • Extract get_metric_filter() as standalone function to build filter
    conditions for a metric
  • Add validation to raise InvalidSearchQuery when multiple different
    metrics are detected
 +134/-37
types.py
Update config interface and centralize types                         

src/sentry/search/eap/types.py

  • Update extra_conditions() signature to accept selected_columns and
    equations parameters
  • Move MetricType type alias definition from trace_metrics config to
    this module
  • Define MetricType as Literal["counter", "gauge", "distribution"]
 +9/-1     
rpc_dataset_common.py
Use centralized filter utilities and new query resolution

src/sentry/snuba/rpc_dataset_common.py

  • Import and_trace_item_filters from new rpc_utils module
  • Update get_table_rpc_request() to call resolve_query_with_columns()
    instead of resolve_query()
  • Remove duplicate and_trace_item_filters() function definition (moved
    to rpc_utils)
  • Update get_timeseries_query() to call resolve_query_with_columns()
    with selected axes and equations
  • Remove manual extra_conditions calls from
    run_top_events_timeseries_query() as they are now handled internally
 +20/-30 
trace_metrics.py
Remove redundant extra conditions handling                             

src/sentry/snuba/trace_metrics.py

  • Remove manual extra_conditions calls from run_table_query() and
    run_timeseries_query()
  • Simplify method signatures by removing extra_conditions parameter from
    TableQuery calls
  • Rely on internal query resolution to handle metric condition injection
 +0/-6     
Tests
test_organization_events_trace_metrics.py
Update tests for metric aggregation changes                           

tests/snuba/api/endpoints/test_organization_events_trace_metrics.py

  • Update test assertions to use - instead of none for metric unit
    parameter in aggregation functions
  • Add three new test cases for embedded metric aggregations
  • Test single metric extraction from aggregation functions
  • Test multiple aggregations with same metric name
  • Test error handling when multiple different metrics are specified
 +74/-11 

Zylphrex and others added 5 commits November 10, 2025 17:05
@Zylphrex
feat(tracemetrics): Parse metric from aggregations
e0dd291 
This parses the metric from aggregations instead of relying on the top level
metric name/type. The top level ones still take precendence for better
compatibility. It has the limitation that it only allows a single metric across
all the aggregations.
@Zylphrex
Merge branch 'master' into txiao/feat/parse-metric-from-aggregations
218e4e5
@Zylphrex
address PR comments
362ad18
@Zylphrex
fix tests
b3aa782
@tomerqodo
Apply changes for benchmark PR
59b06e6
@qodo-code-review
Copy link

qodo-code-review bot commented Dec 4, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
⚪
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

⚪
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing Auditing: The new metric-parsing and condition-injection paths add/alter critical query-building
behavior without adding any audit logging of who performed the action, when, and what was
applied.

Referred Code 
def resolve_query_with_columns(
    self,
    querystring: str | None,
    selected_columns: list[str] | None,
    equations: list[str] | None,
) -> tuple[
    TraceItemFilter | None,
    AggregationFilter | None,
    list[VirtualColumnDefinition | None],
]:
    where, having, contexts = self.resolve_query(querystring)

    # Some datasets like trace metrics require we inject additional
    # conditions in the top level.
    dataset_conditions = self.resolve_dataset_conditions(selected_columns, equations)
    where = and_trace_item_filters(where, dataset_conditions)

    return where, having, contexts

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Broad Exception: In metric extraction from equations an overly broad exception is caught and ignored, which
may hide parsing/validation failures and degrade input validation guarantees.

Referred Code 
try:
    resolved_equation, _ = search_resolver.resolve_equation(equation)
    if isinstance(resolved_equation, ResolvedMetricAggregate):
        if resolved_equation.metric_name and resolved_equation.metric_type:
            metric = Metric(
                metric_name=resolved_equation.metric_name,
                metric_type=resolved_equation.metric_type,
                metric_unit=resolved_equation.metric_unit,
            )
            selected_metrics.add(metric)
except Exception:
    pass

Learn more about managing compliance generic rules or creating your own custom rules
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

qodo-code-review bot commented Dec 4, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid catching and silencing all exceptions

Replace the broad except Exception: pass with except InvalidSearchQuery: pass to
avoid silencing unexpected errors during equation resolution.

src/sentry/search/eap/trace_metrics/config.py [79-93]

 if equations:
     for equation in equations:
         # Attempt to resolve the equation to extract metrics
         try:
             resolved_equation, _ = search_resolver.resolve_equation(equation)
             if isinstance(resolved_equation, ResolvedMetricAggregate):
                 if resolved_equation.metric_name and resolved_equation.metric_type:
                     metric = Metric(
                         metric_name=resolved_equation.metric_name,
                         metric_type=resolved_equation.metric_type,
                         metric_unit=resolved_equation.metric_unit,
                     )
                     selected_metrics.add(metric)
-        except Exception:
+        except InvalidSearchQuery:
+            # This can happen if the equation is not a valid metric aggregation,
+            # which is fine. We only care about valid ones.
             pass
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that catching a broad Exception and silencing it is a bad practice that can hide bugs. Replacing it with a more specific InvalidSearchQuery improves code robustness and maintainability by allowing unexpected errors to surface while still correctly handling expected parsing failures.

Medium
Validate metric type against allowed values

Add a runtime validation check to ensure the metric_type argument is one of the
allowed values ("counter", "gauge", "distribution") before casting.

src/sentry/search/eap/columns.py [403-410]

 if all(
     isinstance(resolved_argument, str) and resolved_argument != ""
     for resolved_argument in resolved_arguments[1:]
 ):
     # a metric was passed
     metric_name = cast(str, resolved_arguments[1])
-    metric_type = cast(MetricType, resolved_arguments[2])
+    metric_type_val = resolved_arguments[2]
+    if metric_type_val not in MetricType.__args__:
+        raise InvalidSearchQuery(f"Invalid metric type '{metric_type_val}'. Must be one of {MetricType.__args__}")
+    metric_type = cast(MetricType, metric_type_val)
     metric_unit = cast(str, resolved_arguments[3])
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly points out a missing runtime validation for metric_type. Adding this check improves the robustness of the code by ensuring only valid metric types are processed, preventing potential downstream errors and providing clearer feedback to the user on invalid input.

Medium
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Review effort 3/5 Scope: Backend

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tomerqodo @Zylphrex